-
-
Notifications
You must be signed in to change notification settings - Fork 7
Linter: create rule to remove other validation keywords when enum is present based on type #1899
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Linter: create rule to remove other validation keywords when enum is present based on type #1899
Conversation
…based on type Signed-off-by: karan-palan <[email protected]>
src/extension/alterschema/linter/enum_validation_keywords_default.h
Outdated
Show resolved
Hide resolved
src/extension/alterschema/linter/enum_validation_keywords_default.h
Outdated
Show resolved
Hide resolved
… string Signed-off-by: karan-palan <[email protected]>
…alidation-keywords
Signed-off-by: karan-palan <[email protected]>
…alidation-keywords
@jviotti any comments? |
Signed-off-by: karan-palan <[email protected]>
…alidation-keywords
Signed-off-by: karan-palan <[email protected]>
src/extension/alterschema/linter/enum_validation_keywords_default.h
Outdated
Show resolved
Hide resolved
src/extension/alterschema/linter/enum_validation_keywords_default.h
Outdated
Show resolved
Hide resolved
src/extension/alterschema/linter/enum_validation_keywords_default.h
Outdated
Show resolved
Hide resolved
"minimum": 10, | ||
"minLength": 2, | ||
"minItems": 1, | ||
"minProperties": 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your tests, mainly for older versions, are not considering the fact that many of these keywords do not exist in such older versions. For example, minProperties
is only present in Draft 4.
The test works because in Draft 0, minProperties
is not considered to be a keyword, therefore it applies to "any" type.
However, to make the tests cleaner, can you double check that you are not using any seemingly valid keyword on a Draft version that doesn't actually define it?
To test the above case, we can also have specific new tests that have a non-sense x-foo-bar
keyword or something like that, and ensure it doesn't get removed
Signed-off-by: karan-palan <[email protected]>
Signed-off-by: karan-palan <[email protected]>
Signed-off-by: karan-palan <[email protected]>
src/extension/alterschema/linter/non_applicable_enum_validation_keywords.h
Outdated
Show resolved
Hide resolved
src/extension/alterschema/linter/non_applicable_enum_validation_keywords.h
Outdated
Show resolved
Hide resolved
Signed-off-by: karan-palan <[email protected]>
…alidation-keywords
Signed-off-by: karan-palan <[email protected]>
Signed-off-by: karan-palan <[email protected]>
src/extension/alterschema/linter/non_applicable_enum_validation_keywords.h
Outdated
Show resolved
Hide resolved
src/extension/alterschema/linter/non_applicable_enum_validation_keywords.h
Outdated
Show resolved
Hide resolved
src/extension/alterschema/linter/non_applicable_enum_validation_keywords.h
Outdated
Show resolved
Hide resolved
src/extension/alterschema/linter/non_applicable_enum_validation_keywords.h
Outdated
Show resolved
Hide resolved
src/extension/alterschema/linter/non_applicable_enum_validation_keywords.h
Outdated
Show resolved
Hide resolved
Signed-off-by: karan-palan <[email protected]>
Signed-off-by: karan-palan <[email protected]>
…alidation-keywords
Signed-off-by: karan-palan <[email protected]>
const sourcemeta::core::SchemaWalker &walker, | ||
const sourcemeta::core::SchemaResolver &) const | ||
-> sourcemeta::core::SchemaTransformRule::Result override { | ||
if (!contains_any(vocabularies, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use the new ONLY_CONTINUE_IF
for every return false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's nice!
return false; | ||
} | ||
|
||
return APPLIES_TO_POINTERS(std::move(positions)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Signed-off-by: karan-palan <[email protected]>
The rule removes: